Skip to content

fix: fail closed on invalid project policy manifests - #242

Merged
LadyBluenotes merged 2 commits into
mainfrom
fix/230-policy-fail-closed
Sep 5, 2026
Merged

fix: fail closed on invalid project policy manifests#242
LadyBluenotes merged 2 commits into
mainfrom
fix/230-policy-fail-closed

Conversation

@LadyBluenotes

@LadyBluenotes LadyBluenotes commented Sep 5, 2026

Copy link
Copy Markdown
Member

🎯 Changes

A malformed project package.json was treated as missing policy, which could turn a deny-all allowlist into migration-mode access to every discovered skill. Policy reads now reject unreadable files, invalid JSON, and non-object manifests with a diagnostic identifying the file. A dangling manifest symlink also fails; genuinely missing manifests retain existing migration behavior.

Regression tests cover core listing/loading, inherited exclusions within a resolved workspace, and CLI list/load/default-install/install-map failures without skill output or generated guidance. Includes a patch changeset and trust-model documentation.

Fixes #230.

Workspace-boundary detection remains separate in #240: malformed npm root JSON can hide the workspace itself. This PR handles confirmed project policy files and inherited policy within a resolved workspace.

✅ Checklist

  • I have followed the steps in the Contributing guide.
  • I have tested this code locally with pnpm run test:pr.

Verification: 588 unit tests and 73 integration tests passed, along with types, ESLint, Knip, Sherif, documentation links, build, and git diff --check. Local PR checks used NX_NO_CLOUD=true, NX_DAEMON=false, a temporary npm cache, and pnpm_config_verify_deps_before_run=false to reuse installed dependencies in the isolated worktree.

🚀 Release Impact

  • This change affects published code, and I have generated a changeset.
  • This change is docs/CI/dev-only (no release).

Summary by CodeRabbit

  • Bug Fixes

    • Policy-controlled skill listing, loading, and installation now stop with a clear error when the project policy manifest is unreadable, malformed, or has an invalid format.
    • Error messages identify the affected manifest path, including inherited project policies.
    • Missing policy manifests continue to support existing migration behavior.
  • Documentation

    • Updated the trust model documentation to explain how missing and invalid policy manifests are handled.

@nx-cloud

nx-cloud Bot commented Sep 5, 2026

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 0956a68

Command Status Duration Result
nx run-many --targets=build ✅ Succeeded <1s View ↗

☁️ Nx Cloud last updated this comment at 2026-09-05 01:37:01 UTC

@pkg-pr-new

pkg-pr-new Bot commented Sep 5, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@tanstack/intent@242

commit: be56d32

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 65483abd-b2c3-47f1-bd33-7c0efd0c2389

📥 Commits

Reviewing files that changed from the base of the PR and between 206e987 and 0956a68.

📒 Files selected for processing (5)
  • .changeset/quiet-policy-errors.md
  • docs/concepts/trust-model.md
  • packages/intent/src/core/package-json.ts
  • packages/intent/tests/core.test.ts
  • packages/intent/tests/integration/source-policy-surfaces.test.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change makes policy manifest handling fail closed for unreadable, malformed, and non-object package.json files. Missing manifests retain migration behavior. Core APIs, CLI surfaces, documentation, and release metadata reflect the updated behavior.

Changes

Fail-closed policy handling

Layer / File(s) Summary
Manifest read and validation
packages/intent/src/core/package-json.ts
readPackageJson returns null only for confirmed missing files. Other read failures, invalid JSON, and non-object JSON values raise path-specific errors.
Core API policy coverage
packages/intent/tests/core.test.ts
Tests cover missing, malformed, non-object, unreadable, dangling-symlink, and inherited malformed manifests for listing and loading.
CLI validation and policy documentation
packages/intent/tests/integration/source-policy-surfaces.test.ts, docs/concepts/trust-model.md, .changeset/quiet-policy-errors.md
CLI tests verify failed commands emit no skill output or generated guidance. Documentation and release metadata describe the fail-closed behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 0956a

Unreadable, malformed, and non-object policy manifests now stop policy-controlled operations rather than exposing skills, while genuinely missing manifests retain migration behavior. Core and CLI coverage supports the intended error and no-output behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address issue #230 by rejecting malformed, unreadable, non-object, and dangling policy manifests; preserving missing-manifest behavior; protecting inherited exclusions; and covering core a…
Out of Scope Changes check ✅ Passed The implementation, tests, changeset, and trust-model documentation support the linked issue objectives. Workspace-boundary discovery remains explicitly outside this pull request's scope.
Description check ✅ Passed The description follows the required template. It explains the change and motivation, completes the checklist, documents release impact, and includes testing details and scope limitations.
Title check ✅ Passed The title is concise and accurately describes the main change: invalid project policy manifests now fail closed.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/230-policy-fail-closed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@LadyBluenotes
LadyBluenotes merged commit 4e59119 into main Sep 5, 2026
9 checks passed
@LadyBluenotes
LadyBluenotes deleted the fix/230-policy-fail-closed branch September 5, 2026 01:39
@github-actions github-actions Bot mentioned this pull request Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fail closed when project policy package.json cannot be read or parsed

1 participant